fix: revoke access token on duplicate auth code user#786
Merged
steveiliop56 merged 3 commits intomainfrom Apr 14, 2026
Merged
Conversation
dosubot
Bot
added
the
size:L
Contributor
📝 WalkthroughWalkthroughAdds tracking of authorization code hashes to persisted OIDC tokens and invalidates tokens tied to a code hash when a code exchange fails (e.g., reused code). Controller now attempts to delete tokens by code hash on code lookup errors before returning the token exchange error response. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as OIDC Controller
participant Service as OIDC Service
participant Repo as Token Repository
participant DB as Database
Client->>Controller: POST /token (authorization_code)
Controller->>Repo: GetCodeEntry(codeHash)
alt code lookup error (not found/expired/invalid_client)
Repo-->>Controller: error
Controller->>Service: DeleteTokenByCodeHash(codeHash)
Service->>Repo: DeleteOidcTokenByCodeHash(codeHash)
Repo->>DB: DELETE FROM oidc_tokens WHERE code_hash = ?
DB-->>Repo: result
Repo-->>Service: OK/error
Service-->>Controller: OK/error
Controller-->>Client: HTTP 400/401 (appropriate token error)
else code valid
Repo-->>Controller: codeEntry
Controller->>Service: CreateOidcToken(..., codeHash)
Service->>Repo: CreateOidcToken(...)
Repo->>DB: INSERT INTO oidc_tokens (..., code_hash, ...)
DB-->>Repo: inserted row
Repo-->>Service: created token
Service-->>Controller: token
Controller-->>Client: HTTP 200 (access_token)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #786 +/- ##
==========================================
- Coverage 20.12% 20.09% -0.04%
==========================================
Files 50 50
Lines 3970 3987 +17
==========================================
+ Hits 799 801 +2
- Misses 3099 3113 +14
- Partials 72 73 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
sql/oidc_schemas.sql (1)
16-16: Consider indexingoidc_tokens.code_hashfor the new revoke path.
DeleteOidcTokenByCodeHashnow uses this column directly; an index avoids full-table scans as token volume grows.Suggested schema addition
CREATE TABLE IF NOT EXISTS "oidc_tokens" ( @@ "nonce" TEXT DEFAULT "" ); + +CREATE INDEX IF NOT EXISTS "idx_oidc_tokens_code_hash" ON "oidc_tokens" ("code_hash");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/oidc_schemas.sql` at line 16, Add a non-unique index on the oidc_tokens.code_hash column to avoid full-table scans when DeleteOidcTokenByCodeHash looks up tokens by code_hash; implement this as a migration that creates the index (use CREATE INDEX CONCURRENTLY IF NOT EXISTS on oidc_tokens(code_hash) so it won’t lock the table) and ensure the migration is applied before deploying the revoke path change.internal/repository/oidc_queries.sql.go (1)
276-283: Add index or uniqueness constraint onoidc_tokens(code_hash).
DeleteOidcTokenByCodeHashwill scan the entireoidc_tokenstable without an index oncode_hash. Without aUNIQUEconstraint, multiple rows could also be deleted if duplicates exist. Consider adding at least an index, ideally aUNIQUEconstraint on this column to match theoidc_codes.code_hashpattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repository/oidc_queries.sql.go` around lines 276 - 283, The DeleteOidcTokenByCodeHash query (deleteOidcTokenByCodeHash / Queries.DeleteOidcTokenByCodeHash) will do a full table scan and may remove multiple rows if duplicates exist; add a database migration to create an index on oidc_tokens(code_hash) and make it UNIQUE (or at minimum a non-unique index) to match the pattern used for oidc_codes.code_hash, run the migration so the schema is updated, and then keep the existing query; if business logic allows multiple tokens per code_hash, document that and leave a non-unique index instead of UNIQUE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/assets/migrations/000008_oidc_coder_user.up.sql`:
- Line 1: The migration adds a nullable "code_hash" column but the schema
expects NOT NULL; update the ALTER TABLE in 000008_oidc_coder_user.up.sql to
create the column with a non-null constraint and a default empty string (e.g.,
ALTER TABLE "oidc_tokens" ADD COLUMN "code_hash" TEXT NOT NULL DEFAULT '';),
ensuring the "oidc_tokens" table and "code_hash" column match
sql/oidc_schemas.sql and migrated databases behave the same as fresh installs.
In `@internal/controller/oidc_controller.go`:
- Around line 278-280: The call to controller.oidc.DeleteTokenByCodeHash(c,
controller.oidc.Hash(req.Code)) is currently ignored; change it to capture the
returned error, check it, and handle failures instead of proceeding silently.
Specifically, assign the result (err :=
controller.oidc.DeleteTokenByCodeHash(...)), and if err != nil: log the error
with context (including the code hash and request info) and return an
appropriate HTTP error response from the handler (or bail out) so the replay
path doesn’t continue with a possibly-valid token; treat a benign "not found"
case differently only if your token store exposes that sentinel error. Ensure
you reference controller.oidc.DeleteTokenByCodeHash,
controller.oidc.Hash(req.Code), the request object req.Code and the request
context c when implementing this.
In `@internal/repository/oidc_queries.sql.go`:
- Line 226: Migration 000008 (and any other migrations that add the "code_hash"
column) currently adds code_hash with DEFAULT "" but no backfill or NOT NULL,
causing runtime scan panics when reading into the non-nullable Go field
i.CodeHash; update the migration SQL that adds "code_hash" to include an
explicit UPDATE to set "" for existing NULLs and then ALTER COLUMN ... SET NOT
NULL (i.e. ADD COLUMN "code_hash" TEXT DEFAULT ""; UPDATE "oidc_tokens" SET
"code_hash" = "" WHERE "code_hash" IS NULL; ALTER TABLE "oidc_tokens" ALTER
COLUMN "code_hash" SET NOT NULL;), and apply the same change to any other
migration snippets that later scan code_hash (references: oidc_queries.sql.go
scanning into &i.CodeHash).
---
Nitpick comments:
In `@internal/repository/oidc_queries.sql.go`:
- Around line 276-283: The DeleteOidcTokenByCodeHash query
(deleteOidcTokenByCodeHash / Queries.DeleteOidcTokenByCodeHash) will do a full
table scan and may remove multiple rows if duplicates exist; add a database
migration to create an index on oidc_tokens(code_hash) and make it UNIQUE (or at
minimum a non-unique index) to match the pattern used for oidc_codes.code_hash,
run the migration so the schema is updated, and then keep the existing query; if
business logic allows multiple tokens per code_hash, document that and leave a
non-unique index instead of UNIQUE.
In `@sql/oidc_schemas.sql`:
- Line 16: Add a non-unique index on the oidc_tokens.code_hash column to avoid
full-table scans when DeleteOidcTokenByCodeHash looks up tokens by code_hash;
implement this as a migration that creates the index (use CREATE INDEX
CONCURRENTLY IF NOT EXISTS on oidc_tokens(code_hash) so it won’t lock the table)
and ensure the migration is applied before deploying the revoke path change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aed6d574-234d-4920-8a02-07e4e77c7c56
📒 Files selected for processing (9)
internal/assets/migrations/000008_oidc_coder_user.down.sqlinternal/assets/migrations/000008_oidc_coder_user.up.sqlinternal/controller/oidc_controller.gointernal/controller/oidc_controller_test.gointernal/repository/models.gointernal/repository/oidc_queries.sql.gointernal/service/oidc_service.gosql/oidc_queries.sqlsql/oidc_schemas.sql
scottmckendry
approved these changes
Apr 11, 2026
dosubot
Bot
added
size:M
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/controller/oidc_controller.go (1)
278-280:⚠️ Potential issue | 🟠 MajorDon’t continue when replay-token revocation fails.
Line 278 logs
DeleteTokenByCodeHashfailures but still proceeds with the original error response. That can leave a replay-associated token active. Please fail closed (returnserver_error) when revocation fails.Suggested fix
- if err := controller.oidc.DeleteTokenByCodeHash(c, controller.oidc.Hash(req.Code)); err != nil { - tlog.App.Error().Err(err).Msg("Failed to delete access token by code hash") - } + codeHash := controller.oidc.Hash(req.Code) + if delErr := controller.oidc.DeleteTokenByCodeHash(c, codeHash); delErr != nil { + tlog.App.Error().Err(delErr).Str("code_hash", codeHash).Msg("Failed to delete access token by code hash") + c.JSON(400, gin.H{ + "error": "server_error", + }) + return + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/oidc_controller.go` around lines 278 - 280, The current code logs failures from controller.oidc.DeleteTokenByCodeHash(c, controller.oidc.Hash(req.Code)) but continues processing, which can leave a replayable token active; change the handler so that if DeleteTokenByCodeHash returns an error you log it (using tlog.App.Error().Err(err).Msg(...)) and then immediately return a server_error response (i.e., fail closed) instead of proceeding with the original error flow; update the code path around DeleteTokenByCodeHash to propagate/return the server_error on revocation failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/controller/oidc_controller.go`:
- Around line 278-280: The current code logs failures from
controller.oidc.DeleteTokenByCodeHash(c, controller.oidc.Hash(req.Code)) but
continues processing, which can leave a replayable token active; change the
handler so that if DeleteTokenByCodeHash returns an error you log it (using
tlog.App.Error().Err(err).Msg(...)) and then immediately return a server_error
response (i.e., fail closed) instead of proceeding with the original error flow;
update the code path around DeleteTokenByCodeHash to propagate/return the
server_error on revocation failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ba7124a-d4da-40b7-8dc6-6138c9087627
📒 Files selected for processing (2)
internal/controller/oidc_controller.gointernal/controller/oidc_controller_test.go
Rycochet
approved these changes
Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests